-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes ios accessibility send focus request to an unfocusable node #25738
Conversation
@@ -284,7 +284,7 @@ - (BOOL)isAccessibilityElement { | |||
// item that is currently off screen but the a11y navigation needs to know | |||
// about. | |||
return (([self node].flags & ~flutter::kScrollableSemanticsFlags) != 0 && | |||
[self node].flags != static_cast<int32_t>(flutter::SemanticsFlags::kIsHidden)) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was a typo.
@@ -329,14 +333,13 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode, | |||
return object; | |||
} | |||
|
|||
SemanticsObject* candidate = nil; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what had got into my head when i wrote this code. The pr change it to be more readable without changing the functionality
Ping @gaaclarke for the review please? |
Also cc @goderbauer. |
a friendly bump |
@@ -284,7 +284,7 @@ - (BOOL)isAccessibilityElement { | |||
// item that is currently off screen but the a11y navigation needs to know | |||
// about. | |||
return (([self node].flags & ~flutter::kScrollableSemanticsFlags) != 0 && | |||
[self node].flags != static_cast<int32_t>(flutter::SemanticsFlags::kIsHidden)) || | |||
([self node].flags & ~static_cast<int32_t>(flutter::SemanticsFlags::kIsHidden)) != 1) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct, why are you checking against the 1 value? Are you trying to check to see if the kIsHidden bit is turned on? It should look like what is above in that case: ([self node].flags & flutter::kScrollableSemanticsFlags) != 0
Checking anything against 1 is going to be fragile since it would depend on which bit is kIsHidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, I made a mistake, I surprised this still work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. The bitwise math looks a bit off. If it is right, it is fragile because of what a I mentioned in the comment. It occurred to me that maybe you wanted to check that it wasn't set? That would be like this: ([self node].flags & flutter::kScrollableSemanticsFlags) == 0
new flutter::MockAccessibilityBridge()); | ||
fml::WeakPtr<flutter::AccessibilityBridgeIos> bridge = factory.GetWeakPtr(); | ||
flutter::SemanticsNode node; | ||
node.flags = static_cast<int32_t>(flutter::SemanticsFlags::kHasImplicitScrolling) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these casts necessary? They should already be int32_t's. (fwiw I think people use unsigned ints for flags to avoid 2's compliment weirdness)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this error if i get rid of the static cast
error: invalid operands to binary expression ('flutter::SemanticsFlags' and 'flutter::SemanticsFlags')
node.flags = flutter::SemanticsFlags::kHasImplicitScrolling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird, thanks
if (nextToFocus) { | ||
nextToFocus = FindFirstFocusable(nextToFocus); | ||
} else if (root) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you do these 2 checks I think it would be helpful if you pulled this out into a function like:
Object* FindFirstFocusable(Object* one, Object* two) {
if (one) {
return FindFirstFocusable(one);
} else if (two) {
return FindFirstFocusable(two);
}
return nullptr;
}
This function is a monster, the more duplicate logic we can pull out the better imo.
Logic seems find though.
@gaaclarke Thanks for reviewing, I addressed all comments, this is ready for another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix!
When we build a list view, we use cache extent to build some hidden element outside the view port so the voiceover can focus it and showOnScreen to move the hidden element into the screen to achieve scrolling. There is an issue if that hidden off screen element does not have any label/value/hint and thus not focusable when they are on screen. the accessibility bridge will still send the layoutchange to focus that element when it is scrolled to the viewport because it was previously focused. This PR fixes it so that when it send focus request, it will check again if that element is accessibility element and finds the first focusable child of the element if it is not. If none of the child is focusable, the layoutchange will be sent with nil target which lets the voiceover to decide what to focus next.
fixes flutter/flutter#80991
Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.